feat: add onlyDiff in options by Ayc0 · Pull Request #317 · americanexpress/jest-image-snapshot

Ayc0 · 2022-11-08T12:19:12Z

Description

Add a new option onlyDiff in toMatchImageSnapshot

Motivation and Context

When working with complex rules, we may want to handle the 3 images independently: the received image, the diff image (with only the diff), and the expected image.

How Has This Been Tested?

I added a unit test
And also tested it with this screenshot:

Without `onlyDiff` (or with `onlyDiff = false`)	With `onlyDiff = true`

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (adding or updating documentation)
Dependency update

Checklist:

My change requires a change to the documentation and I have updated the documentation accordingly.
My changes are in sync with the code style of this project.
There aren't any other open Pull Requests for the same issue/update.
These changes should be applied to a maintenance branch.
I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using Jest-Image-Snapshot?

CLAassistant · 2022-11-08T12:19:21Z

All committers have signed the CLA.

Ayc0 · 2022-11-08T12:29:57Z

+function composeDiff(options) {
+  const {
+    diffDirection, baselineImage, diffImage, receivedImage, imageWidth, imageHeight, onlyDiff,
+  } = options;
+  const composer = new ImageComposer({
+    direction: diffDirection,
+  });
+
+  if (!onlyDiff) {
+    composer.addImage(baselineImage, imageWidth, imageHeight);
+  }
+  composer.addImage(diffImage, imageWidth, imageHeight);
+  if (!onlyDiff) {
+    composer.addImage(receivedImage, imageWidth, imageHeight);
+  }
+  return composer;


I created a new function, otherwise the complexity of diffImageToSnapshot was too high

Ayc0 · 2022-11-08T13:44:11Z

@code-forger I pushed an update

Ayc0 · 2022-11-14T13:46:19Z

@code-forger can you have another look? 🙏

Ayc0 · 2022-11-23T08:42:22Z

@10xLaCroixDrinker can you have a look? 🙏

10xLaCroixDrinker · 2022-11-29T23:09:48Z

+      expect(diffResult).toHaveProperty('pass', false);
+      // White image without the baseline, nor the received (white because we mock pixelmatch)
+      expect(diffResult).toHaveProperty('imgSrcString', 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAGQAAABkCAYAAABw4pVUAAAA30lEQVR4Ae3BIQEAAACDMAT9M78G4ptcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcilyKXIpcygA9UAGR3m7UvwAAAABJRU5ErkJggg==');
+      expect(diffResult).toHaveProperty('diffOutputPath', path.join(mockSnapshotsDir, '__diff_output__', `${mockSnapshotIdentifier}-diff.png`));


These assertions don't give me any confidence. Can we add something that clearly checks that only the diff is included?

What if I add to the stubs folder the expected result of test instead of doing a manual data:image/png;base64,…?

Edit: I removed this test and I moved it to an integration test, so that we can have a real check and a real diff image

Ayc0 · 2022-12-02T09:12:32Z

@10xLaCroixDrinker do you know if we also need @code-forger's approval (as a "request changes" was applied)?

# [6.1.0](v6.0.0...v6.1.0) (2022-12-02) ### Features * add onlyDiff in options ([#317](#317)) ([4bad752](4bad752))

oneamexbot · 2022-12-02T16:47:11Z

🎉 This PR is included in version 6.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Ayc0 · 2022-12-02T17:38:04Z

Thanks! 🎉

Ayc0 · 2022-12-06T08:50:48Z

FYI I'm also changing the types for this new option in DefinitelyTyped/DefinitelyTyped#63501

@Ayc0

…n version 6.1.0 by @Ayc0 * [jest-image-snapshot] add `onlyDiff` option Add changes introduced in americanexpress/jest-image-snapshot#317 to the types * [jest-image-snapshot] update version reference * [jest-image-snapshot] add reference to author Ayc0 * [jest-image-snapshot] add tests for onlyDiff

# [6.1.0](americanexpress/jest-image-snapshot@v6.0.0...v6.1.0) (2022-12-02) ### Features * add onlyDiff in options ([americanexpress#317](americanexpress#317)) ([4bad752](americanexpress@4bad752))

feat: add onlyDiff in options

bcb2a67

github-actions Bot added the one-app-team-review-requested label Nov 8, 2022

Ayc0 marked this pull request as ready for review November 8, 2022 12:28

Ayc0 requested a review from a team as a code owner November 8, 2022 12:28

Ayc0 commented Nov 8, 2022

View reviewed changes

code-forger requested changes Nov 8, 2022

View reviewed changes

Comment thread __tests__/__snapshots__/index.spec.js.snap Outdated

Comment thread src/index.js Outdated

code-forger requested a review from a team November 8, 2022 12:46

refactor: set default value for onlyDiff to false

9a79167

Ayc0 requested a review from code-forger November 8, 2022 14:24

10xLaCroixDrinker suggested changes Nov 29, 2022

View reviewed changes

Ayc0 added 2 commits November 30, 2022 09:48

refactor: apply Jamie King PR suggestion

3ad314c

chore: move test for diffOnly to an integration test

40e6c11

Ayc0 requested review from 10xLaCroixDrinker and code-forger and removed request for 10xLaCroixDrinker and code-forger November 30, 2022 08:54

10xLaCroixDrinker approved these changes Nov 30, 2022

View reviewed changes

Ayc0 requested a review from code-forger November 30, 2022 15:00

code-forger approved these changes Dec 2, 2022

View reviewed changes

10xLaCroixDrinker merged commit 4bad752 into americanexpress:main Dec 2, 2022

oneamexbot added a commit that referenced this pull request Dec 2, 2022

chore(release): 6.1.0 [skip ci]

722dd55

# [6.1.0](v6.0.0...v6.1.0) (2022-12-02) ### Features * add onlyDiff in options ([#317](#317)) ([4bad752](4bad752))

oneamexbot added the released label Dec 2, 2022

Ayc0 deleted the only-diff branch December 2, 2022 17:38

Ayc0 mentioned this pull request Dec 6, 2022

[jest-image-snapshot] add onlyDiff option added in version 6.1.0 DefinitelyTyped/DefinitelyTyped#63501

Merged

8 tasks

goverdhan07 pushed a commit to goverdhan07/jest-image-snapshot that referenced this pull request Jul 23, 2023

feat: add onlyDiff in options (americanexpress#317)

6667006

Conversation

Ayc0 commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

What is the Impact to Developers Using Jest-Image-Snapshot?

Uh oh!

CLAassistant commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ayc0 Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ayc0 commented Nov 8, 2022

Uh oh!

Ayc0 commented Nov 14, 2022

Uh oh!

Ayc0 commented Nov 23, 2022

Uh oh!

Uh oh!

10xLaCroixDrinker Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

Ayc0 Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

Ayc0 Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

Ayc0 commented Dec 2, 2022

Uh oh!

oneamexbot commented Dec 2, 2022

Uh oh!

Ayc0 commented Dec 2, 2022

Uh oh!

Ayc0 commented Dec 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ayc0 commented Nov 8, 2022 •

edited

Loading

CLAassistant commented Nov 8, 2022 •

edited

Loading